feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

sviderock · 2024-10-06T21:00:14Z

Description

2nd PR for RET-1207. Implements the following list of basic Transactions Feed functionality:

Adds fetching of all the pages for the wallet address up to the point when there are no transaction to fetch anymore (not showing "no transactions" toast yet, will be added in the follow-up PR).
Cursor for the next page is the timestamp of the last transaction from the current page. If next page includes the same transaction - it gets deduplicated.
Re-uses pending transactions from pendingStandByTransactionsSelector
Polls first page every 10 seconds
Sorts transactions to show approvals at the top if the corresponding transaction has identical timestamp

More comments for different parts of the pagination flow are added as comments in the file in the next PR.

Test plan

10 out of 16 tests from TransactionFeed.test.ts were re-used and adjusted to the new data fetching flow. Other tests will be added in the follow-up PRs purely for the sake of trying to keep the PRs smaller.

Related issues

Relates to RET-1207

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-10-07T09:31:01Z

Codecov Report

Attention: Patch coverage is 88.88889% with 15 lines in your changes missing coverage. Please review.

Project coverage is 88.76%. Comparing base (fb7f7dd) to head (1226b03).
Report is 1 commits behind head on slava/add-rtk-query.

Files with missing lines	Patch %	Lines
src/transactions/feed/TransactionFeedV2.tsx	88.28%	14 Missing and 1 partial ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           slava/add-rtk-query    #6135    +/-   ##
=====================================================
  Coverage                88.75%   88.76%            
=====================================================
  Files                      731      733     +2     
  Lines                    30827    30972   +145     
  Branches                  5630     5358   -272     
=====================================================
+ Hits                     27362    27493   +131     
- Misses                    3266     3434   +168     
+ Partials                   199       45   -154

Files with missing lines	Coverage Δ
src/transactions/NoActivity.tsx	`100.00% <ø> (ø)`
src/transactions/api.ts	`100.00% <100.00%> (+20.00%)`	⬆️
src/transactions/apiTestHelpers.ts	`100.00% <ø> (ø)`
src/transactions/reducer.ts	`85.21% <100.00%> (ø)`
src/transactions/types.ts	`97.82% <ø> (ø)`
src/transactions/utils.ts	`96.92% <100.00%> (+0.09%)`	⬆️
src/transactions/feed/TransactionFeedV2.tsx	`88.28% <88.28%> (ø)`

... and 66 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb7f7dd...1226b03. Read the comment docs.

sviderock · 2024-10-08T15:23:16Z

src/transactions/feed/TransactionFeedV2.tsx

+          // eslint-disable-next-line react-hooks/rules-of-hooks
+          pageData: useMemo(
+            () => ({
+              currentCursor: result.originalArgs?.endCursor, // timestamp from the last transaction from the previous page.


These will be changed to use pageInfo data from the blockchain-api response once the pagination-related PR is merged.

sviderock · 2024-10-08T15:25:40Z

src/transactions/feed/TransactionFeedV2.tsx

+        return {
+          ...result,
+          // eslint-disable-next-line react-hooks/rules-of-hooks
+          pageData: useMemo(


I've added a comment explaining the usage of this useMemo and some other parts in the next PR

ok but this is a bit disturbing to me for now 😅
It's not yet clear to me that it's always called.
And not messing something up with what React expects for hooks.

jeanregisser

Awesome! 🎉

Approved with some suggestions / questions.

jeanregisser · 2024-10-09T12:58:41Z

src/transactions/feed/TransactionFeedV2.tsx

+}
+
+// Query poll interval
+const POLL_INTERVAL = 10000 // 10 secs


nit:

Suggested change

const POLL_INTERVAL = 10000 // 10 secs

const POLL_INTERVAL_MS = 10_000 // 10 secs

jeanregisser · 2024-10-09T13:08:48Z

src/transactions/feed/TransactionFeedV2.tsx

+        return {
+          ...result,
+          // eslint-disable-next-line react-hooks/rules-of-hooks
+          pageData: useMemo(


ok but this is a bit disturbing to me for now 😅
It's not yet clear to me that it's always called.
And not messing something up with what React expects for hooks.

jeanregisser · 2024-10-09T13:20:11Z

src/transactions/feed/TransactionFeedV2.tsx

+}
+
+function renderItem({ item: tx }: { item: TokenTransaction; index: number }) {
+  switch (tx.__typename) {


Could we use tx.type instead?
__typename is something added by GraphQL and I wanted to avoid relying on it with the new implementation.

@jeanregisser this will introduce a small drop in type-safety and will require some extra type casting. Shouldn't be a problem for now but potentially something that we might want to revisit once we move to TransactionFeedV2 completely.

jeanregisser · 2024-10-09T13:28:44Z

src/transactions/feed/TransactionFeedV2.tsx

+    { skip: !address, pollingInterval: POLL_INTERVAL }
+  )
+
+  useEffect(() => {


nit: related to https://valora-app.slack.com/archives/C025V1D6F3J/p1728479551405369

Suggested change

useEffect(() => {

useEffect(function updatePaginatedData() {

jeanregisser · 2024-10-09T13:32:26Z

src/transactions/feed/TransactionFeedV2.tsx

+import { walletAddressSelector } from 'src/web3/selectors'
+
+type PaginatedData = {
+  [timestamp: number]: TokenTransaction[]


Should we type it like this? In case we read it with a timestamp that wasn't added?

Suggested change

[timestamp: number]: TokenTransaction[]

[timestamp: number]: TokenTransaction[] | undefined

@jeanregisser I still plan to introduce the data persistence (with some workarounds) to have that instant first page load as we have on the existing transaction feed. But I agree - this should include undefined while the persistence is absent.

Or alternatively, instead of this:

const [paginatedData, setPaginatedData] = useState<PaginatedData>({})

I can define it like this:

const [paginatedData, setPaginatedData] = useState<PaginatedData>({ [FIRST_PAGE_TIMESTAMP]: [] })

The first page will always be refreshed anyways so it can be empty from the start.

jeanregisser · 2024-10-09T13:34:48Z

src/transactions/feed/TransactionFeedV2.test.tsx

+  })
+})
+
+describe('TransactionFeed', () => {


Suggested change

describe('TransactionFeed', () => {

describe('TransactionFeedV2', () => {

jeanregisser · 2024-10-09T13:48:20Z

src/transactions/feed/TransactionFeedV2.test.tsx

+    expect(tree.getByText('feedItemFailedTransaction')).toBeTruthy()
+  })
+
+  it('tries to fetch 20 transactions, unless the end is reached', async () => {


Maybe this should be the name of the new test?
And there would be another one checking it fetches the first page initially?

Suggested change

it('tries to fetch 20 transactions, unless the end is reached', async () => {

it('fetches the next page by scrolling to the end of the list', async () => {

I don't think we need the previous behavior of automatically loading the next page
that we added in #2779 which is why this name for the test was used.

Right?

@jeanregisser

Sorry for the confusion! I've already renamed this and the next test in the next PR

I've also already added similar test in the next PR

There's no behaviour for fetching next pages if the current one is absent. Probably me keeping most of the old tests' names is the reason for the confusion. I'll revisit all the test names in the follow-up PR!

jeanregisser · 2024-10-09T13:48:44Z

src/transactions/feed/TransactionFeedV2.test.tsx

+  it('tries to fetch 10 transactions, and stores empty pages', async () => {
+    mockFetch
+      .mockResponseOnce(typedResponse({ transactions: [mockTransaction()] }))
+      .mockResponseOnce(typedResponse({ transactions: [] }))
+
+    const { store, ...tree } = renderScreen()
+
+    await store.dispatch(
+      transactionFeedV2Api.endpoints.transactionFeedV2.initiate({ address: '0x00', endCursor: 0 })
+    )
+    await store.dispatch(
+      transactionFeedV2Api.endpoints.transactionFeedV2.initiate({ address: '0x00', endCursor: 123 })
+    )
+
+    await waitFor(() => tree.getByTestId('TransactionList'))
+
+    await waitFor(() => expect(mockFetch).toHaveBeenCalledTimes(2))
+    expect(getNumTransactionItems(tree.getByTestId('TransactionList'))).toBe(1)
+  })


Do we really need this one?

@jeanregisser I think you're right that we can skip this. In the next PR I've added a test that checks for correct lazy-load of a few pages of data so that should cover the same thing this test was covering.

jeanregisser · 2024-10-09T13:50:25Z

src/transactions/feed/TransactionFeedV2.tsx

+  useTransactionFeedV2Query(
+    { address: address!, endCursor: FIRST_PAGE_TIMESTAMP },
+    { skip: !address, pollingInterval: POLL_INTERVAL }
+  )


If I understand correctly, this will cause the other hook to return new data?
That's why we're not reading the result from it?

@jeanregisser Correct. Multiple usages of the same hook with the same arguments will retrieve the data from single cached data point within the api reducer that library manages itself.

So we can even call this hook in multiple places to retrieve data and if at any moment that data updates anywhere – all the usages will instantly get updated with the new data.
The same goes for this polling example. It's task is only to trigger the fetch every 10 sec and once the data is updated - the hook above with the selectFromResult will trigger all the necessary changes and re-renders.

sviderock · 2024-10-09T15:30:55Z

@jeanregisser Updated as per the comments! Also, found a bug in logic of updating the pagination data for the first page so I've updated that as well.

### Description 3rd PR for RET-1207. It merges existing confirmed (failed/completed) transaction from the `standByTransactions` reducer into each paginated data. ### Test plan WIP ### Related issues - Relates to RET-1207 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

sviderock added 3 commits October 6, 2024 23:07

add initial implementation from Transaction Feed V2

03bcdec

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

26ba6e8

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

fa90827

sviderock changed the base branch from main to slava/add-rtk-query October 7, 2024 12:49

sviderock added 3 commits October 7, 2024 15:52

Add tests for TransactionFeedV2

4695304

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

221b0d1

remove unnecessary calls from tests

6adf3ed

sviderock marked this pull request as ready for review October 7, 2024 13:33

sviderock added 3 commits October 8, 2024 14:04

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

64b7db8

remove redundant file

7206027

fix wrong useMemo dependencies for sections

78e7239

sviderock commented Oct 8, 2024

View reviewed changes

sviderock added 4 commits October 9, 2024 11:22

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

95027c4

revert knip fixes

97bbe92

Merge branch 'slava/add-rtk-query' into slava/transaction-feed-v2

f105d15

refactor .reduce usage for readability

f5ab01e

jeanregisser approved these changes Oct 9, 2024

View reviewed changes

sviderock added 2 commits October 9, 2024 18:28

remove unnecessary test

7e747f3

fix polling data was not updating, fix as per review comments

bb11791

sviderock merged commit 2dad608 into slava/add-rtk-query Oct 11, 2024
11 checks passed

sviderock deleted the slava/transaction-feed-v2 branch October 11, 2024 07:41

sviderock changed the title ~~feat(feed): Add initial implementation of Transaction Feed V2~~ feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

sviderock commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

sviderock Oct 8, 2024 •

edited

Loading

sviderock Oct 8, 2024

jeanregisser Oct 9, 2024

jeanregisser left a comment

jeanregisser Oct 9, 2024

jeanregisser Oct 9, 2024

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

jeanregisser Oct 9, 2024

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

jeanregisser Oct 9, 2024

jeanregisser Oct 9, 2024 •

edited

Loading

sviderock Oct 9, 2024 •

edited

Loading

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

sviderock commented Oct 9, 2024

	const POLL_INTERVAL = 10000 // 10 secs
	const POLL_INTERVAL_MS = 10_000 // 10 secs

	[timestamp: number]: TokenTransaction[]
	[timestamp: number]: TokenTransaction[] \| undefined

	describe('TransactionFeed', () => {
	describe('TransactionFeedV2', () => {

	it('tries to fetch 20 transactions, unless the end is reached', async () => {
	it('fetches the next page by scrolling to the end of the list', async () => {

feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

feat(TransactionFeedV2): Add initial implementation of Transaction Feed V2 #6135

Conversation

sviderock commented Oct 6, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

sviderock Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

sviderock Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Oct 9, 2024

sviderock commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

sviderock Oct 8, 2024 •

edited

Loading

jeanregisser Oct 9, 2024 •

edited

Loading

sviderock Oct 9, 2024 •

edited

Loading